gh-93896: allow enabling asyncio.Runner calls to asyncio.set_event_loop independantly to loop_factory#94058
Conversation
Lib/asyncio/runners.py
Outdated
There was a problem hiding this comment.
this opts asyncio.Runner out of the policy system and asyncio.run opts back in
f21f808 to
abbd6b4
Compare
| If set_policy_loop is True, the event loop in the default policy will be | ||
| set. |
There was a problem hiding this comment.
If this is classified as a bug (clearly it is causing issues), then fixing it shouldn't be blocking. By only implementing this behind an argument we are introducing a new feature which means that this will only be added to 3.12 (and not 3.11).
I would vote for not adding this argument.
There was a problem hiding this comment.
I think it's important to maintain the asyncio.Runner support for a high level policy-free use of asyncio, so that the policy system can be deprecated and removed
There was a problem hiding this comment.
By adding this option we make it more difficult to deprecate the policy system because then this option will need to be deprecated as well. Users who specify this option will have more to do to migrate.
There was a problem hiding this comment.
How about I remove the flag from asyncio.Runner and have callers use a wrapper that mutates the policy loop?
@contextmanager
def _with_policy_loop(loop):
try:
asyncio.set_event_loop(loop)
yield
finally:
asyncio.set_event_loop(None)runner = asyncio.Runner()
cmgr = _with_policy_loop(runner.get_loop())
try:
cmgr(runner.run)(corofn())
finally:
cmgr(runner.close)()Users who specify this option will have more to do to migrate.
I'm proposing this set_policy_loop flag get deprecated at the same time as the policy system. I could add the flag as deprecated in this PR so users won't add it unless they already have more work to do to migrate from the policy system as a concept
There was a problem hiding this comment.
I don't think I understand the downside with enabling this by default. My arguments boil down to:
-
With this flag disabled, we retain the current behaviour which causes issues. I would guess that most people don't fully understand what it does and will leave it to the default, then potentially run into these issues.
-
As we've so far discussed, this flag is a fix for an issue which might soon no matter much because it is planned to be deprecated. With this option needing to be explicitly enabled, we introduce more things to deprecate. If, instead, this would be enabled by default then most would not need to change anything as it gets removed after the deprecation period.
I am not super confident in the event loop policy system, so I won't continue arguing if you don't agree with me after this comment - i'll defer to whenever a core dev takes a look at this issue.
There was a problem hiding this comment.
With this flag disabled, we retain the current behaviour which causes issues. I would guess that most people don't fully understand what it does and will leave it to the default, then potentially run into these issues.
It should be opt in for asyncio.Runner
There was a problem hiding this comment.
yep: set_policy_loop defaults to False in asyncio.Runner: def __init__(self, *, debug=None, loop_factory=None, set_policy_loop=False):
people using the old asyncio.run get the old behaviour that uses the policy loop factory and sets the policy loop. People using the new asyncio.Runner get the new behaviour which does not use the policy loop factory or set the policy loop by default
| This function always creates a new event loop from the default policy sets | ||
| it in the event loop policy and closes it at the end and sets the policy | ||
| loop to None. |
There was a problem hiding this comment.
This long sentence is quite wordy and without knowing the change I would probably not understand what it means.
I would probably suggest not making this change (referring to this very sentence). To most, setting the loop in the event loop policy is probably synonymous to actually creating and running a loop.
Misc/NEWS.d/next/Library/2022-06-21-13-08-46.gh-issue-93896.2BsYCX.rst
Outdated
Show resolved
Hide resolved
…sYCX.rst Co-authored-by: Bluenix <bluenixdev@gmail.com>
|
I've merged the changes from #94593 so I'm now proposing this as a feature change instead of a bug fix |
| if self._set_policy_loop: | ||
| events.set_event_loop(None) |
There was a problem hiding this comment.
@kumaraditya303 it looks like your PR is missing some calls to set_event_loop here
There was a problem hiding this comment.
asyncio.Runner should be used a context manager and its close method removes the set event loop so not required here.
There was a problem hiding this comment.
the issue being when Runner.run finishes the loop should be unset
with Runner() as runner1, Runner() as runner2:
runner1.run(fn()) # calls set_event_loop
runner2.run(fn()) # also calls set_event_loop
# I'd expect the event loop to be unset hereThere was a problem hiding this comment.
I'd expect the event loop to be unset here
That happens only if you supply your loop_factory otherwise for existing code, its better to keep it set for existing code not using asyncio.Runner.
There was a problem hiding this comment.
I would expect the event loop to be set until the context manager exit which is what we have currently.
There was a problem hiding this comment.
Ah I see what you mean
|
I'll close this PR for now. It might make sense to re-introduce the |
Fixes #93896